fix: Enforce aggregate MaximumAmount in multi-send MPT#6644
fix: Enforce aggregate MaximumAmount in multi-send MPT#6644
Conversation
rippleSendMultiMPT used a read-only SLE snapshot (view.read) to check MaximumAmount per iteration. Since rippleCreditMPT updates a separate mutable copy (view.peek), the snapshot's sfOutstandingAmount was stale after the first iteration, allowing the aggregate to exceed MaximumAmount. Replace the per-iteration check with a running total that validates the aggregate against MaximumAmount within the send loop. The old per-iteration check is retained behind a !fixAssortedFixes gate for ledger replay compatibility.
Tapanito
added
Amendment
AI Triage
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…-send-max-amount # Conflicts: # include/xrpl/protocol/detail/features.macro # src/libxrpl/ledger/View.cpp
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Went through the changes
The core fix is correct, but four issues need attention: a uint64_t wrap-around risk in the pre-amendment arithmetic (line 1215), a semantic divergence from the original subtraction-underflow behavior that may break ledger replay fidelity (line 1215), missing // KNOWN BUG comments at the broken-behavior assertion sites in the test (line 3351), and a reminder to queue fixSecurity3_1_3 for prompt activation and audit other multi-send paths for the same stale-snapshot pattern. See inline comments.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6644 +/- ##
=======================================
Coverage 81.4% 81.5%
=======================================
Files 998 998
Lines 74443 74450 +7
Branches 7563 7558 -5
=======================================
+ Hits 60632 60640 +8
+ Misses 13811 13810 -1
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Use subtraction-based guards instead of addition to prevent uint64_t overflow in both the post-amendment aggregate check and the pre-amendment per-iteration check. Each condition in the cascade protects the subtraction in the next from underflow. Move totalSendAmount accumulation after the check so the guard operates on the pre-addition value.
There was a problem hiding this comment.
Looked through this one
One high-severity security note flagged inline: the pre-fixSecurity3_1_3 path allows issuer to bypass MaximumAmount via stale snapshot — activate the amendment promptly.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
There was a problem hiding this comment.
Pull request overview
Fixes a correctness gap in MPToken multi-destination sends where MaximumAmount could be exceeded because per-iteration checks observed a stale view.read() snapshot instead of the updated outstanding amount.
Changes:
- Update
rippleSendMultiMPTto enforceMaximumAmountusing an aggregate/running-total check inside the send loop (with pre-amendment behavior retained for replay compatibility). - Add a unit test covering multi-send aggregate
MaximumAmountenforcement, including a pre-amendment “known bug” case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/libxrpl/ledger/helpers/TokenHelpers.cpp |
Implements aggregate MaximumAmount enforcement for issuer multi-send, with amendment gating for replay compatibility. |
src/test/app/MPToken_test.cpp |
Adds a regression test validating correct aggregate enforcement and preserving pre-amendment behavior expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Number totalSendAmount; | ||
| auto const maximumAmount = sle->at(~sfMaximumAmount).value_or(maxMPTokenAmount); | ||
| auto const outstandingAmount = sle->getFieldU64(sfOutstandingAmount); | ||
|
|
||
| // actual accumulates the total cost to the sender (includes transfer | ||
| // fees for third-party transit sends). takeFromSender accumulates only | ||
| // the transit portion that is debited to the issuer in bulk after the | ||
| // loop. They diverge when there are transfer fees. | ||
| STAmount takeFromSender{mptIssue}; | ||
| actual = takeFromSender; | ||
|
|
||
| for (auto const& r : receivers) | ||
| for (auto const& [receiverID, amt] : receivers) | ||
| { | ||
| auto const& receiverID = r.first; | ||
| STAmount amount{mptIssue, r.second}; | ||
| STAmount const amount{mptIssue, amt}; | ||
|
|
||
| if (amount < beast::zero) | ||
| { | ||
| return tecINTERNAL; // LCOV_EXCL_LINE | ||
| } | ||
|
|
||
| /* If we aren't sending anything or if the sender is the same as the | ||
| * receiver then we don't need to do anything. | ||
| */ | ||
| if (!amount || (senderID == receiverID)) | ||
| if (!amount || senderID == receiverID) | ||
| continue; | ||
|
|
||
| if (senderID == issuer || receiverID == issuer) | ||
| { | ||
| // if sender is issuer, check that the new OutstandingAmount will | ||
| // not exceed MaximumAmount | ||
| if (senderID == issuer) | ||
| { | ||
| XRPL_ASSERT_PARTS( | ||
| takeFromSender == beast::zero, | ||
| "xrpl::rippleSendMultiMPT", | ||
| "sender == issuer, takeFromSender == zero"); | ||
|
|
||
| auto const sendAmount = amount.mpt().value(); | ||
| auto const maximumAmount = sle->at(~sfMaximumAmount).value_or(maxMPTokenAmount); | ||
| if (sendAmount > maximumAmount || | ||
| sle->getFieldU64(sfOutstandingAmount) > maximumAmount - sendAmount) | ||
| return tecPATH_DRY; | ||
|
|
||
| if (view.rules().enabled(fixSecurity3_1_3)) | ||
| { | ||
| // Post-fixSecurity3_1_3: aggregate MaximumAmount | ||
| // check. Each condition guards the subtraction | ||
| // in the next to prevent underflow. | ||
| auto const exceedsMaximumAmount = | ||
| // This send alone exceeds the max cap | ||
| sendAmount > maximumAmount || | ||
| // The aggregate of all sends exceeds the max cap | ||
| totalSendAmount > maximumAmount - sendAmount || | ||
| // Outstanding + aggregate exceeds the max cap | ||
| outstandingAmount > maximumAmount - sendAmount - totalSendAmount; | ||
|
|
||
| if (exceedsMaximumAmount) | ||
| return tecPATH_DRY; | ||
| totalSendAmount += sendAmount; |
There was a problem hiding this comment.
totalSendAmount is tracked as Number, but this MaximumAmount enforcement needs exact integer arithmetic. Number precision depends on mantissa scale (small vs large), and in small scale values near maxMPTokenAmount can lose unit precision, potentially allowing a small aggregate overflow or false rejection. Consider using an exact integral type here (e.g. std::uint64_t or MPTAmount) and keep the comparisons/subtractions in unsigned 63-bit arithmetic, similar to the existing per-iteration check.
| if (view.rules().enabled(fixSecurity3_1_3)) | ||
| { | ||
| // Post-fixSecurity3_1_3: aggregate MaximumAmount | ||
| // check. Each condition guards the subtraction | ||
| // in the next to prevent underflow. | ||
| auto const exceedsMaximumAmount = | ||
| // This send alone exceeds the max cap | ||
| sendAmount > maximumAmount || | ||
| // The aggregate of all sends exceeds the max cap | ||
| totalSendAmount > maximumAmount - sendAmount || | ||
| // Outstanding + aggregate exceeds the max cap | ||
| outstandingAmount > maximumAmount - sendAmount - totalSendAmount; | ||
|
|
||
| if (exceedsMaximumAmount) | ||
| return tecPATH_DRY; | ||
| totalSendAmount += sendAmount; | ||
| } | ||
| else | ||
| { | ||
| // Pre-fixSecurity3_1_3: per-iteration MaximumAmount | ||
| // check. Reads sfOutstandingAmount from a stale | ||
| // view.read() snapshot — incorrect for multi-destination | ||
| // sends but retained for ledger replay compatibility. | ||
| if (sendAmount > maximumAmount || | ||
| outstandingAmount > maximumAmount - sendAmount) | ||
| return tecPATH_DRY; |
There was a problem hiding this comment.
The PR description mentions retaining the old behavior behind a !fixAssortedFixes gate, but the implementation is gated on fixSecurity3_1_3. Since fixAssortedFixes doesn't appear to exist in the codebase, please update the PR description (or rename the gate if that was the intent) to avoid confusion for reviewers and future archaeology.
rippleSendMultiMPT used a read-only SLE snapshot (view.read) to check MaximumAmount per iteration. Since rippleCreditMPT updates a separate mutable copy (view.peek), the snapshot's sfOutstandingAmount was stale after the first iteration, allowing the aggregate to exceed MaximumAmount.
Replace the per-iteration check with a running total that validates the aggregate against MaximumAmount within the send loop. The old per-iteration check is retained behind a !fixAssortedFixes gate for ledger replay compatibility.
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)